-
Notifications
You must be signed in to change notification settings - Fork 1k
client: make CLIENT_QUERY_ATTRIBUTES opt-out to restore proxy compatibility #1041
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Adjust client capability flags based on server support, and account for explicitly disabled capabilities. - client/auth.go: Update client capability flags - client/conn.go: Add field to manage disabled capabilities
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This pull request implements an opt-out mechanism for the CLIENT_QUERY_ATTRIBUTES capability to restore proxy compatibility with legacy MySQL servers. The key changes include:
- Adding a new field (dcaps) to track explicitly disabled capabilities.
- Updating the SetCapability and UnsetCapability functions to modify both ccaps and dcaps.
- Revising the capability negotiation logic in writeAuthHandshake to conditionally advertise CLIENT_QUERY_ATTRIBUTES.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| client/conn.go | Introduced dcaps and updated SetCapability/UnsetCapability functions. |
| client/auth.go | Adjusted capability inheritance logic in writeAuthHandshake. |
Comments suppressed due to low confidence (2)
client/conn.go:48
- [nitpick] Consider renaming 'dcaps' to 'disabledCaps' for improved clarity.
dcaps uint32 // disabled capabilities
client/auth.go:215
- [nitpick] Optionally, consider renaming 'inherit' to 'inheritedCaps' to more clearly convey its purpose.
inherit := c.capability & ^c.dcaps // Server-side capabilities minus explicit denies
lance6716
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm at the first glance. If you are not urgent, I want to check if there are other potential problems before merging this PR.
dveeden
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems fine to me on first glance
client/auth.go
Outdated
| capability |= c.capability & mysql.CLIENT_LONG_FLAG | ||
| capability |= c.capability & mysql.CLIENT_QUERY_ATTRIBUTES | ||
| // ---- Inherit capabilities that the server has and the user has NOT explicitly denied ---- | ||
| inherit := c.capability & ^c.dcaps // Server-side capabilities minus explicit denies |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe name this inheritedCaps or inherited?
client/conn.go
Outdated
| capability uint32 | ||
| // client-set capabilities only | ||
| ccaps uint32 | ||
| dcaps uint32 // disabled capabilities |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see you followed the naming style of ccaps. Just a weak comment: capability, ccaps and dcaps are hard to understand at their usage functions. Maybe rename them to serverCaps, clientExplicitOnCaps, clientExplicitOffCaps or something.
| mysql.CLIENT_LONG_PASSWORD | mysql.CLIENT_TRANSACTIONS | mysql.CLIENT_PLUGIN_AUTH | ||
| // Adjust client capability flags based on server support | ||
| capability |= c.capability & mysql.CLIENT_LONG_FLAG | ||
| capability |= c.capability & mysql.CLIENT_QUERY_ATTRIBUTES |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we don't need to specially process CLIENT_QUERY_ATTRIBUTES. There are 3 states of capability:
- developer calls
SetCapability, which means theccapsbit is 1 and thedcapsbit is 0 - developer calls
UnsetCapability, which meansccapsbit is 0 and thedcapsbit is 1 - No calls, which means both bits are 0
So here we turn on the bits by ccaps at around line 219, and have another round to turn off bits by dcaps. I suggest
capability |= c.ccaps&mysql.CLIENT_FOUND_ROWS | ...
capability &= c.dcaps... // ⬅️ here we add CLIENT_QUERY_ATTRIBUTES and other flags from above line to turn off
Adjust client capability flags based on server support. Add a new field to manage explicitly disabled flags. - client/auth.go: Improve logic for adjusting client capability flags - client/conn.go: Add field to manage explicitly disabled flags
2nd round update — why this patch? 🚀Thanks for the quick feedback! ✨ What changed
All other capability flags inherit the same three-state behaviour automatically. 📝 Why this shape?
Let me know if you spot anything I’ve missed! |
Background — why proxies built with this SDK break
masahide/mysql8-audit-proxy#8
A common architecture for a MySQL audit / filter / router built on go-mysql looks like this:
Downstream side (
go-mysql/server)Implements the old 5.7/8.0 protocol and does not recognise
CLIENT_QUERY_ATTRIBUTES.When an application connects, the handshake therefore has the bit cleared.
Upstream side (
go-mysql/client≥ v1.12.0)In
writeAuthHandshake()the client automatically copiesCLIENT_QUERY_ATTRIBUTESfrom the server greeting, claiming the proxywill send query attributes.
Mismatch
The proxy simply forwards the raw
COM_QUERYit received from theapplication.
Because the downstream never added the attributes section, the packet sent
upstream is missing mandatory fields → MySQL returns
ERROR 1835 (HY000) Malformed communication packet.What the proxy author needs
A way to say “don’t advertise
CLIENT_QUERY_ATTRIBUTESon the upstreamconnection even if the server supports it, because my downstream side can’t
handle that mode.”
Currently that is impossible:
UnsetCapability()removes the flag fromccaps, butwriteAuthHandshake()puts it back from the server’s flag set.Patch overview
Add
dcaps(“deny caps”) toclient.Connto track flags explicitlydisabled by the caller.
Modify
SetCapability/UnsetCapability:In
writeAuthHandshake():Store the final negotiated set back into
c.capabilityfor later use.Resulting behaviour
UnsetCapability(CLIENT_QUERY_ATTRIBUTES)SetCapability(CLIENT_QUERY_ATTRIBUTES)Example — disabling the flag in a proxy
With this one-liner the proxy once again works with both modern MySQL servers
and the existing
go-mysql/serverfront end.Safety & compatibility
caller explicitly opts out.
go-mysql/server.